-
Notifications
You must be signed in to change notification settings - Fork 678
Bug 1148743 - Add button to open samples in JSFiddle #3147
Conversation
We can add a line to forms.styl which adds the margin to any button after the first button and then we can localize it for right to left display as well. .open-in-host + .open-in-host { |
|
||
function create_jsfiddle(title, htmlCode, cssCode, jsCode) { | ||
var $form = $('<form method="post" target="_blank" action="https://jsfiddle.net/api/post/library/pure/">' | ||
+ '<input type="hidden" id="html" name="html" />' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need any of these id
attributes? These seem unnecessary. Instead of $form.find('#html')
, can you do $form.find('input[name=html')
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought id
would be better performance-wise, although in this case since we have very few elements - it shouldn't make much difference.
Wanted to add: super excited to see a PR for this. It's a feature everyone has been talking about for a while! |
17ed6b2
to
aad10d5
Compare
Good for another review :) I also made minor change in |
Do we want to insert the buttons using optimizely? |
IIRC, @openjck wanted to use optimizely to change the order of the buttons. So we may not need to insert them via optimizely, but we may run an Optimizely experiment that re-orders them for 50% of viewers. |
Having had some time to think about it, I would recommend just measuring clicks with Google Analytics. Switching the order of the buttons with Optimizely (or switching between showing one and showing another) would be bad for habituation. If one button is clicked way more than the other, we could decide to support only that service. If the buttons receive similar numbers of clicks, we could decide to support both services. I don't expect button placement will have a very significant effect on number of clicks. edit: Ah, but of course Optimizely and Waffle bucket users only once and keep users in their buckets between reloads. (At least in theory. I notice that the current homepage heading Waffle doesn't persist buckets between reloads.) In that case, we could use Optimizely to show one button or the other and measure which one gets more clicks. In that case, inserting the buttons with Optimizely probably would be the easier approach. That would mitigate the effect of any FOUCs. I don't expect the numbers will be significantly different, but it wouldn't hurt to try it out. |
|
||
.open-in-host + .open-in-host { | ||
bidi-style(margin-left, ($grid-spacing / 2), margin-right, 0); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a line break at end of file (picky GitHub is picky).
All my comments have been addressed, thank you :) It would be nice if the sample code in the commit message was also updated. |
I'm definitely up for doing an A/B test. I'll coordinate with @iakshay offline to get it going. |
Updated code to insert buttons function create_buttons() {
var section = $(this).find('iframe').attr('id').substring("frame_".length);
return '<div style="margin-bottom:10px;"><button class="open-in-host" data-host="jsfiddle" data-section="' +
section + '"><i aria-hidden="true" class="icon-jsfiddle"></i> Open in JSFiddle</i></button>'
+ '<button class="open-in-host" data-host="codepen" data-section="' +
section + '"><i aria-hidden="true" class="icon-codepen"></i> Open in Codepen</button></div>';
}
$('.sample-code-frame').parent().prepend(create_buttons);
$('.sample-code-table').before(create_buttons); |
var $sample = $('<div />').append(sample); | ||
var htmlCode = $sample.find('.brush\\:.html, .brush\\:.html\\;').text(); | ||
var cssCode = $sample.find('.brush\\:.css, .brush\\:.css\\;').text(); | ||
var jsCode = $sample.find('.brush\\:.js, .brush\\:.js\\;').text(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@darkwing Will these selectors match all our code sample snippets?
I have written selectors for the following 2 variations
// Case 1
<pre class="brush: js"></pre>
// Case 2
<pre class="brush: js; highlight:[7]"></pre>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I'm not sure if converting code to base64 is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering the same thing. What do you think, @darkwing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is not - see example in https://jsfiddle.net/zalun/mk29ttc2/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will these selectors match all our code sample snippets?
12:09 <davidwalsh> I believe so
12:09 <davidwalsh> I think taht's right
How would you feel about writing the insertion code into the repository, so that we would run something like The code is non-trivial, so it makes sense for us to revision it. We also want to be sure that it stays synchronized with other changes happening in the codebase. |
Maybe we could even add the buttons, but use |
I have created // insert JSFiddle button
mdn.insertSampleButtons(['jsfiddle']);
// insert JSFiddle and Codepen buttons
mdn.insertSampleButtons(['jsfiddle', 'codepen']);
// other order
mdn.insertSampleButtons(['codepen', 'jsfiddle']); I can hide them by default, but this way you can experiment with order also :) |
What would be a good page to spot-check this on? I'm having trouble finding pages that use Since this involves the live samples system, which @darkwing has done the most work with, I think it would be great to get his opinion too. |
}); | ||
|
||
function createSampleButtons(section, sites) { | ||
var buttons = '<div style="margin-bottom:10px;">'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't inline styles -- a CSS class should be created.
ping @darkwing - are you able to review this again? |
@iakshay - are you able to waffle this? |
@groovecoder I'm not sure, how we waffle features client side. Do I just move the code to new file? |
We use a |
Do we need to Waffle this? Even on this branch, the buttons need to be manually injected with JS. |
This does need to be waffled, yes. |
any update? |
Superceded by: #3240 |
Code to insert buttons using javascript.